This seems to make the command recognition example much more robust, although we want to enable it from time to time to make sure that multi-decoder grammar usage is not broken.

Seems the reason for this to improve the results is that the fallback logic depends on the average logprob of the generated sequences:

https://github.com/ggerganov/whisper.cpp/blob/b8f34d1ed786194d9787b1f1d086b89136e361f3/whisper.cpp#L5166-L5171

Therefore, I think both "best of" and "beam search" strategies would not work as expected.
We probably need to have some re-normalization of the logprobs after applying the grammar to make this compatible with these.

In any case, I think for now we should focus on greedy sampling without fallbacks and improve later if needed

I think this makes more sense, though I'm still experimenting.
At least for the use case where we want to strongly restrict the output to match the grammar, this works well with large penalty (>100.0)

Yeah, this seems to work well enough - I've been testing this code with 10 - 15 on tiny. I was hoping for a sweet spot that would guide the decoding towards valid strings while leaving obviously non-matching strings unchanged. But so far, I've found that the point that gives satisfying grammar matching still seems too eager to match unrelated words to the grammar.

I suppose both variants to fail in some way for different use cases. This penalty is essentially a free-parameter which it's not obvious how to set. A more general way would be to let the user provide a penalty callback function and let them implement the penalization, but I guess for now lets keep it simple

But so far, I've found that the point that gives satisfying grammar matching still seems too eager to match unrelated words to the grammar.

Does the original scaling work better in this scenario?

Does the original scaling work better in this scenario?

Anecdotally, no, not really.

Seems we have an issue with EOT handling.
Here I try to penalize EOT if the grammar allows continuation and this seems to help, although it's ugly.

There's actually the start of that, commented out, at the top of the function. I was just unsure initially if that penalty made sense in whisper land.

The decoded string is complete according to the grammar if some stack is empty, so the following will penalize EOT if the decoded string is an incomplete parse. The grammar might still allow continuation after a complete parse, so if we want to only penalize EOT if there's no continuation at all, I guess we'd switch the check to find a nonempty stack.

diff --git a/whisper.cpp b/whisper.cpp
index 5e3b86a..e796ddd 100644
--- a/whisper.cpp
+++ b/whisper.cpp
@@ -3872,13 +3872,13 @@ static void whisper_suppress_invalid_grammar(
         return;
     }
 
-    // bool allow_eot = false;
-    // for (const auto & stack : grammar.stacks) {
-    //     if (stack.empty()) {
-    //         allow_eot = true;
-    //         break;
-    //     }
-    // }
+    bool allow_eot = false;
+    for (const auto & stack : grammar.stacks) {
+        if (stack.empty()) {
+            allow_eot = true;
+            break;
+        }
+    }
 
     std::vector<std::pair<std::vector<uint32_t>, whisper_partial_utf8>> candidates_decoded;
     std::vector<whisper_grammar_candidate>                              candidates_grammar;
@@ -3898,10 +3898,8 @@ static void whisper_suppress_invalid_grammar(
         logprobs[reject.id] -= params.grammar_penalty;
     }
 
-    // when the grammar does not allow any continuation, we don't want to penalize the EOT token
-    // TODO: is there are better way to do this?
-    printf("rejects.size() = %zu, whisper_token_eot(&ctx) - 2 = %d\n", rejects.size(), whisper_token_eot(&ctx) - 2);
-    if ((int) rejects.size() < whisper_token_eot(&ctx) - 2) {
+    // penalize EOT if grammar is incomplete
+    if (!allow_eot) {
         logprobs[whisper_token_eot(&ctx)] -= params.grammar_penalty;
     }
     //fprintf(stderr, "Allowed: (%zu tokens)\n", size - rejects.size());

These are irrelevant from the grammar functionality - I think improve the transcription in general.
Not sure why OpenAI don't have these filters in the original implementation (or maybe they do?)

Moving this down here improves the results in my tests significantly. The idea is that we keep the logic for sampling a timestamp token intact, irrespective of what the grammar suggests. Only after we have made the decision about the next token being text, we apply the grammar filters.

Makes sense! This was my first foray into whisper internals so I don't have a nuanced understanding of the decoding process.